Fix used-before-assignment for PEP 695 type aliases + parameters #10488
Conversation
cdce8p
added
typing
False Positive 🦟
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10488 +/- ##
=======================================
Coverage 95.87% 95.87%
=======================================
Files 176 176
Lines 19168 19170 +2
=======================================
+ Hits 18377 18379 +2
Misses 791 791
🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
| @@ -21,6 +21,7 @@ | |||
| "ext", | |||
| "regression", | |||
| "regression_02", | |||
| "used_02", | |||
There was a problem hiding this comment.
Sometimes I wonder if it still makes sense to keep the artificial file limit around. IMO it's just annoying most of the time. Should we remove it?
pylint/pylint/testutils/functional/find_functional_tests.py
Lines 13 to 17 in 32304fe
There was a problem hiding this comment.
Without the file limit it was really annoying to find a functional tests though (scrolling for eternity). It's not going to be annoying for a while if we remove the file limit contraint though. Might be a release time check maybe ?
There was a problem hiding this comment.
Without the file limit it was really annoying to find a functional tests though (scrolling for eternity).
I mostly use Go to File ... in VS Code so I can't speak to that.
My issue here was a more practical one. I wanted to add one more test which should be placed alongside the other used_before_assignment test cases but when I did it immediately triggered limit and I wasn't even able to continue working on the test case. Sure I could move a lot of tests around here but that would just inflate the diff. Furthermore, moving tests makes git blame a lot more difficult. Sure there is .git-blame-ignore-revs but it only contains one commit.
It's not going to be annoying for a while if we remove the file limit contraint though. Might be a release time check maybe ?
That would probably just delay the issue and make releases more time consuming.
There was a problem hiding this comment.
Do you never just browse the functional tests to see if something match what you want to test ? I agree with the git blame not working well if we move a lot of file. (It's already the case almost everywhere, I did refactor a lot in the past, and I'm not alone in 20 years, some git blame in pylint already make me feel like a cyber-crime forensic. I'm not saying we should make it worse voluntarily though).
I'm a little reluctant to remove all rules. One possible solution would be to find another clear organization for the functional tests (like in the extension functional tests directory, probably). One directory per message, avoid testing multiple message in the same file. Or remove constraint on regression directory only : the issue always arise in this directory, right ? Or remove constraint on regression directory but force regression tests to be inside tests/functional/regression/ with a file name being an int as in tests/functional/regression/8736.py. What do you think ?
There was a problem hiding this comment.
Do you never just browse the functional tests to see if something match what you want to test ?
Tbh not really. I usually just search for the error message and see which existing tests pop up or which tests fail if I change something 😅
I agree with the git blame not working well if we move a lot of file.
Yeah, it's broken already basically. Thankfully it still mostly works for the checkers themselves which is usually enough to find the right PR.
I'm a little reluctant to remove all rules. One possible solution would be to find another clear organization for the functional tests (like in the extension functional tests directory, probably). One directory per message, avoid testing multiple message in the same file.
We mostly do that already but sometimes it's just more practical to test multiple different things in one go. Otherwise we end up duplicating a lot of test code which will make maintaining it just more difficult.
Or remove constraint on regression directory only : the issue always arise in this directory, right ? Or remove constraint on regression directory but force regression tests to be inside
tests/functional/regression/with a file name being an int as intests/functional/regression/8736.py. What do you think ?
The unused_variable and used_before_assignment folders are also frequent issues, so it's not just the regression dirs. If it were just one test file, that would be fine. However, with #10382 there are now tests with 3 or even 4 supporting files. Maybe a first small step could be to ignore these and only count .py files?
--
We don't need to find a particular solution for the issue here. It was just something that bothered me, so I wanted to highlight it and start the discussion.
There was a problem hiding this comment.
Sure, but isn't the discussion's goal to find a solution to the annoyance ;) ?
| @@ -21,6 +21,7 @@ | |||
| "ext", | |||
| "regression", | |||
| "regression_02", | |||
| "used_02", | |||
There was a problem hiding this comment.
Without the file limit it was really annoying to find a functional tests though (scrolling for eternity). It's not going to be annoying for a while if we remove the file limit contraint though. Might be a release time check maybe ?
Description
Closes #9815